Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#27 cache the volta inventory to reduce times tools need to be downloaded #137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cgrabmann
Copy link

@cgrabmann cgrabmann commented Oct 4, 2023

Fixes #27

This uses the @actions/cache package provided to cache the inventory (cache) of Volta. So that installing the tools needed for the build can work even if the download would be down.

I was triggered to do this by the instability of the node-download-servers in resent weeks.

Since I have no expertise in how exactly Volta works internally and therefore am not 100% sure if this is enough for Volta to work with, I would greatly appreciate feedback.
Ideas on how to cover this with tests, are also very welcome.

Inner workings:
I try to clean up the Volta inventory during teardown and only keep the tools that where actually installed during the run. When uploading the cache I create a cash-key based on the content of the inventory, so that a change in it's content also results in new and updated cache entry.

Limitations right now:
As far as I can tell, the cache is shared across all workflows in a repository. Therefore a project with pipelines all installing different tools might lead to continuous outdated cache entries, since the newest one will be pulled during setup, no matter the hash-suffix.

@cgrabmann cgrabmann changed the title #27: cache the volta inventory to reduce times tools need to be downloaded #27 cache the volta inventory to reduce times tools need to be downloaded Oct 5, 2023
@rwjblue
Copy link
Collaborator

rwjblue commented Jan 15, 2024

Oh no! I'm very sorry for missing this PR. Reviewing now...

@@ -19,7 +19,7 @@
"scripts": {
"build": "npm-run-all build:clean build:ts build:dist build:docs",
"build:clean": "rimraf dist lib",
"build:dist": "ncc build src/index.ts",
"build:dist": "ncc build src/index.ts && ncc build src/cache.ts -o dist-post",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's split this into separate tasks, then use npm-run-all in build:dist. Something like:

Suggested change
"build:dist": "ncc build src/index.ts && ncc build src/cache.ts -o dist-post",
"build:dist": "npm-run-all build:dist:*",
"build:dist:main": "ncc build src/index.ts",
"build:dist:post": "ncc build src/cache.ts -o dist-post",

@@ -19,7 +19,7 @@
"scripts": {
"build": "npm-run-all build:clean build:ts build:dist build:docs",
"build:clean": "rimraf dist lib",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add dist-post here also?

import * as core from '@actions/core';
import * as stateKeys from './state-keys';

async function run(): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets call this main (it makes more sense in my head as main, since it's basically just the mechanism to execute this script itself and still be able to use async/await).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can call this post.ts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we call the other file post.ts, we can call this one src/cache.ts, what do you think?

);
}

function createToolFilePatter(tool: Tool): string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function createToolFilePatter(tool: Tool): string {
function createToolFilePattern(tool: Tool): string {

@@ -0,0 +1 @@
export const VOLTA_HOME = 'VOLTA_HOME';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just use the string where we need it (instead of using this constant).

}
}

async function cleanInventory(voltaHome: string, installedTools: Tool[]): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully grok how this is going to work to actually prune the inventory. If we call restoreInventory in the setup flow of the main action all of the tools & inventory will be copied over from prior runs, then (eventually) call cleanInventory in the post script for the action -- how will we ever prune things out?

Can you walk me through the high level details?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use @action/cache to cache where possible
2 participants